Fix wolfsshd's cleared umask and the SFTP attribute encoder - #1269
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are coherent with the stated Fenrir findings, are narrowly scoped, and add targeted regression tests to prevent reintroducing the corrected behaviors.
Review effort: Lite
Findings: None
What changed in this PR
This PR addresses four independent Fenrir findings across SFTP attribute handling, wolfsshd configuration parsing/logging, daemon umask behavior, and algorithm-list regression coverage, improving protocol correctness, operational safety, and test assurance.
Changes:
- SFTP: make the attribute encoder emit an extension count of zero when extensions are flagged but not implemented, preventing peers from parsing past the attribute block.
- wolfsshd: warn (instead of silently accepting) several OpenSSH-compatible-but-unimplemented config keywords, while still allowing startup with typical
sshd_configfiles. - Tests: add regression coverage for SFTP attribute encode/decode agreement and for default algorithm lists excluding SHA-1 / AES-CBC unless explicitly opted in.
| File | Description |
|---|---|
wolfssh/wolfsftp.h |
Exposes internal-test entry points for SFTP attribute encode/decode under WOLFSSH_TEST_INTERNAL. |
src/wolfsftp.c |
Ensures EXT attributes encode a zero extension-count; adds internal-test wrappers for real encoder/parser. |
tests/regress.c |
Adds a round-trip regression test validating SFTP attribute encoder/parser agreement (including EXT flag behavior). |
tests/api.c |
Adds a test asserting default algo lists exclude weak algorithms unless opt-in macros enable them; includes exact-token list matcher helper. |
apps/wolfsshd/wolfsshd.c |
Sets a non-zero daemon umask (configurable via WOLFSSHD_DEFAULT_UMASK) so sessions don’t inherit a cleared mask. |
apps/wolfsshd/configuration.c |
Warns when encountering recognized-but-unimplemented sshd_config keywords rather than accepting silently. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ejohnstown
changed the title
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1269
Scan targets checked: wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1269
Fenrir already completed a review of this PR at commit dd641bf9ad74 (run 2721); its findings are the review threads on the PR. Push new commits to get a re-review of what changed, or comment @wolfSSL-Fenrir-bot review force to run the full review again at this commit.
SFTP_SetAttributes() sizes and writes only the count for a WOLFSSH_FILEATRB_EXT block, never the records themselves, so the count goes out as zero. A caller's atr->extCount would promise records that do not follow, and the peer's parser reads past the block looking for them. - advance idx past the count, so a later field cannot land on it - wolfSSH_TestSftpSetAttributes() and wolfSSH_TestSftpParseAttributes() reach the encoder and parser for testing - tests/regress.c round-trips the flag combinations Issue: F-2479
Subsystem, ChallengeResponseAuthentication, UsePAM, X11Forwarding, PrintMotd, AcceptEnv and UseDNS parse and are then dropped. An unknown keyword is fatal, so accepting these in silence reads as support for a setting that is not enforced. Log a warning naming the keyword instead. Rejecting them would fail every config copied from OpenSSH. - OptionName() maps an option tag back to its keyword for the message Issue: F-2868
Daemonizing cleared the umask and nothing set one afterwards, so every per-connection child inherited it: files created from a shell or through the SCP receive path came out 0666, directories 0777. Hold 022, which is what a foreground daemon already inherits from its caller. - WOLFSSHD_DEFAULT_UMASK sets it at build time Issue: F-3671
Existing coverage asserts only that the canned KEX, host key, cipher and MAC lists are non-null and that a session inherits them. Read the lists, so an inverted guard cannot put SHA-1 or AES-CBC back into the default proposal unnoticed. - weak entries are expected only under their opt-in macros - modern entries are expected present, so an emptied list still fails Issue: F-13961
philljj
approved these changes
Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four unrelated Fenrir items, one commit each. The wolfsshd umask is the one with an operational effect: every session inherited a cleared mask, so files created from a shell or through SCP came out world-writable.
Both new tests were confirmed to fail with their fix reverted. make check passes on macOS and Linux at each commit, and the wolfsshd suite is green.